Add ShellParams protocol #772

JohnAZoidberg · 2023-04-28T08:12:22Z

Useful to get the shell arguments for a commandline application.

Running example application on UEFI shell:

Shell> fs0:EFI\Boot\BootX64.efi
Args: ["FS0:\\EFI\Boot\BootX64.efi"]
Num args: 1
Shell> fs0:EFI\Boot\BootX64.efi foo bar
Args: ["FS0:\\EFI\Boot\BootX64.efi", "foo", "bar"]
Num args: 3
First real arg: 'foo'
Shell>

Checklist

Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
Update the changelog (if necessary)

JohnAZoidberg · 2023-04-28T08:14:23Z

Hmm, either need to have the test runner not run this test. Or need to run it from a shell.

uefi/src/proto/shell_params.rs

nicholasbishop · 2023-04-29T15:41:35Z

Hmm, either need to have the test runner not run this test. Or need to run it from a shell.

The question of how to run tests that rely on the shell came up in #596 too (cc @timrobertsdev). I haven't looked into it myself and don't have a good answer for now.

I think what you've done in this PR of putting it in a separate file under examples/ might be a good compromise for now though; it will get compiled, but not run automatically by anything, so it's OK that we don't have an env set up for it.

nicholasbishop · 2023-04-29T15:42:57Z

uefi-test-runner/examples/shell_params.rs

@@ -0,0 +1,60 @@
+// ANCHOR: all


These ANCHOR comments are just used by mdbook (see https://rust-osdev.github.io/uefi-rs/HEAD/how_to/protocols.html#walkthrough for example), so unless you are looking to add this example to the book, you can leave these out.

I think it would be good to have a shell application example in the book.
However it would be more useful once the shell protocol is merged.

uefi-test-runner/examples/shell_params.rs

nicholasbishop · 2023-04-29T15:47:12Z

uefi/src/proto/shell_params.rs

+    /// Get a Vec of the shell parameter arguments
+    #[cfg(feature = "alloc")]
+    #[must_use]
+    pub fn get_args(&self) -> Vec<String> {


Rather than collecting into a Vec here, could we return an iterator? That would allow it to work without alloc, and if the user actually needs a Vec they can call .collect() on it.

Oh that's a good idea!

Do you think it would be useful to add something like this as well?

/// Get a slice of the args, as Char16 pointers pub fn get_args_slice(&self) -> &[*const Char16] { unsafe { from_raw_parts(self.argv, self.argc) } }

I'd suggest not exposing that as pub fn unless we have a specific real-world need for the API. Most of the uefi-rs API tries to provide safe wrappers rather than exposing raw pointers.

What about returning a slice or an iterator to CStr16?
I couldn't figure out how to return a &[CStr16], not sure if that's possible.
But an Iterator<Item = CStr16> would be doable. Or do you generally just want to return utf8 Strings only?

I couldn't figure out how to return a &[CStr16], not sure if that's possible.

Does this help?

https://docs.rs/uefi/0.20.0/uefi/data_types/struct.CStr16.html#method.from_ptr

Hmm but can I turn a &[*const Char16] into a &[CStr16]?
Not without copying, right?

nicholasbishop · 2023-06-10T17:47:02Z

Hmm, either need to have the test runner not run this test. Or need to run it from a shell.

We've landed a change to the test-runner so that it runs under the UEFI shell. So you should be able to add runtime tests now :)

JohnAZoidberg · 2023-06-27T05:33:29Z

Awesome! I'll try that, thanks!

uefi-test-runner/examples/shell_params.rs

JohnAZoidberg · 2023-08-21T06:06:42Z

We've landed a change to the test-runner so that it runs under the UEFI shell. So you should be able to add runtime tests now :)

Added! :D

As I understand, currently all tests are run by a single executable, test_runner.efi.
It's called by the shell, so I just tested for those shell parameters.

For more testes I created an example PR: #922. Not sure if you like how I run the other tests.

JohnAZoidberg · 2023-08-31T06:00:59Z

uefi/src/proto/shell_params.rs

+    pub fn get_args(&self) -> impl Iterator<Item = String> {
+        unsafe {
+            from_raw_parts(
+                self.0.argv.cast::<*const data_types::chars::Char16>(),


I don't know why but the compiler thinks argv is *const *const u16 instead of *const *const Char16.

The type of Char16 is defined differently in uefi-raw vs uefi. uefi-raw just uses a u16 so any value is accepted, whereas in uefi we use the ucs2 crate that places some restrictions on the valid character values. (At some point I'd like to improve this situation, see #809 (comment) for more background.)

All of which is to say, this is expected :)

nicholasbishop · 2023-09-04T16:02:55Z

uefi-raw/src/protocol/shell_params.rs

+#[derive(Debug)]
+#[repr(C)]
+pub struct ShellParametersProtocol {
+    /// Pointer to a list of arguments


nit: period at ends of comments.

nicholasbishop · 2023-09-04T16:06:56Z

uefi/src/proto/shell_params.rs

+    pub fn get_args(&self) -> impl Iterator<Item = String> {
+        unsafe {
+            from_raw_parts(
+                self.0.argv.cast::<*const data_types::chars::Char16>(),


The type of Char16 is defined differently in uefi-raw vs uefi. uefi-raw just uses a u16 so any value is accepted, whereas in uefi we use the ucs2 crate that places some restrictions on the valid character values. (At some point I'd like to improve this situation, see #809 (comment) for more background.)

All of which is to say, this is expected :)

nicholasbishop · 2023-09-04T16:18:49Z

uefi/src/proto/shell_params.rs

+impl ShellParameters {
+    /// Get an iterator of the shell parameter arguments
+    #[cfg(feature = "alloc")]
+    pub fn get_args(&self) -> impl Iterator<Item = String> {


naming nit: drop the get_ prefix on these methods (https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter)

nicholasbishop · 2023-09-04T16:22:05Z

uefi/src/proto/shell_params.rs

+
+impl ShellParameters {
+    /// Get an iterator of the shell parameter arguments
+    #[cfg(feature = "alloc")]


Most of our APIs return CStr16s rather than converting to String. (Since there's a good chance that in real usage you'll pass these strings back into other uefi-rs methods, it makes sense to avoid the round-trip conversion from UCS-2 to UTF-8 and back). So this can change to:

pub fn args(&self) -> impl Iterator<Item = &CStr16> { self.get_args_slice() .iter() .map(|x| unsafe { CStr16::from_ptr(*x) }) }

And doesn't need to be gated behind alloc then.

Ok sure, that's a good idea.

nicholasbishop · 2023-09-04T16:23:15Z

uefi/src/proto/shell_params.rs

+
+    /// Get a slice of the args, as Char16 pointers
+    #[must_use]
+    pub fn get_args_slice(&self) -> &[*const Char16] {


Along with changing the other method to be available without alloc, I think we can drop the pub from this method and keep it as an internal helper.

nicholasbishop · 2023-09-04T16:24:22Z

uefi-test-runner/src/proto/shell_params.rs

+    for arg in shell_params.get_args_slice() {
+        let arg_str = unsafe { CStr16::from_ptr(*arg) };
+        info!("  '{}'", arg_str);
+    }


nit: Since the result of getting the args is fully checked below, let's drop printing the args (many of our tests do print stuff, but it's only really necessary in cases where the output can ever change).

Useful to get the shell arguments for a commandline application. Signed-off-by: Daniel Schaefer <[email protected]>

Signed-off-by: Daniel Schaefer <[email protected]>

nicholasbishop · 2023-09-11T01:37:13Z

Lgtm, thanks!

JohnAZoidberg force-pushed the shell-params branch from 031fefb to eed9274 Compare April 28, 2023 08:13

JohnAZoidberg commented Apr 28, 2023

View reviewed changes

uefi/src/proto/shell_params.rs Outdated Show resolved Hide resolved

nicholasbishop reviewed Apr 29, 2023

View reviewed changes

uefi-test-runner/examples/shell_params.rs Outdated Show resolved Hide resolved

nicholasbishop reviewed Apr 29, 2023

View reviewed changes

nicholasbishop mentioned this pull request May 8, 2023

Make UEFI shell protocols testable #793

Merged

2 tasks

JohnAZoidberg force-pushed the shell-params branch from a9e69b3 to fa9f26c Compare August 21, 2023 02:49

JohnAZoidberg commented Aug 21, 2023

View reviewed changes

uefi-test-runner/examples/shell_params.rs Outdated Show resolved Hide resolved

JohnAZoidberg mentioned this pull request Aug 21, 2023

ShellParams: Add subshell test #922

Closed

JohnAZoidberg force-pushed the shell-params branch 2 times, most recently from 1c4e7b6 to 2247e23 Compare August 31, 2023 05:58

JohnAZoidberg commented Aug 31, 2023

View reviewed changes

JohnAZoidberg requested review from phip1611 and nicholasbishop August 31, 2023 06:01

JohnAZoidberg force-pushed the shell-params branch from 2247e23 to cdbc0ef Compare August 31, 2023 06:06

nicholasbishop requested changes Sep 4, 2023

View reviewed changes

JohnAZoidberg added 3 commits September 5, 2023 10:58

ShellParams: Add protocol

ab19c99

Useful to get the shell arguments for a commandline application. Signed-off-by: Daniel Schaefer <[email protected]>

ShellParams: Add example app

5a443ee

Signed-off-by: Daniel Schaefer <[email protected]>

uefi-test-runner: Add test for ShellParams protocol

2c9185b

Signed-off-by: Daniel Schaefer <[email protected]>

JohnAZoidberg force-pushed the shell-params branch from cdbc0ef to 2c9185b Compare September 5, 2023 02:58

nicholasbishop approved these changes Sep 11, 2023

View reviewed changes

nicholasbishop added this pull request to the merge queue Sep 11, 2023

Merged via the queue into rust-osdev:main with commit eb33785 Sep 11, 2023
12 checks passed

fnetz mentioned this pull request Dec 10, 2023

Request for EFI_SHELL_INTERFACE #448

Open

JohnAZoidberg deleted the shell-params branch June 25, 2024 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ShellParams protocol #772

Add ShellParams protocol #772

JohnAZoidberg commented Apr 28, 2023 •

edited

Loading

JohnAZoidberg commented Apr 28, 2023

nicholasbishop commented Apr 29, 2023

nicholasbishop Apr 29, 2023

JohnAZoidberg May 2, 2023

nicholasbishop Apr 29, 2023

JohnAZoidberg May 2, 2023

nicholasbishop May 2, 2023

JohnAZoidberg May 2, 2023

phip1611 May 2, 2023 •

edited

Loading

JohnAZoidberg Aug 21, 2023

nicholasbishop commented Jun 10, 2023

JohnAZoidberg commented Jun 27, 2023

JohnAZoidberg commented Aug 21, 2023

JohnAZoidberg Aug 31, 2023

nicholasbishop Sep 4, 2023

nicholasbishop Sep 4, 2023

nicholasbishop Sep 4, 2023

nicholasbishop Sep 4, 2023

nicholasbishop Sep 4, 2023

JohnAZoidberg Sep 5, 2023

nicholasbishop Sep 4, 2023

nicholasbishop Sep 4, 2023

nicholasbishop commented Sep 11, 2023

Add ShellParams protocol #772

Add ShellParams protocol #772

Conversation

JohnAZoidberg commented Apr 28, 2023 • edited Loading

Checklist

JohnAZoidberg commented Apr 28, 2023

nicholasbishop commented Apr 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phip1611 May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholasbishop commented Jun 10, 2023

JohnAZoidberg commented Jun 27, 2023

JohnAZoidberg commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholasbishop commented Sep 11, 2023

JohnAZoidberg commented Apr 28, 2023 •

edited

Loading

phip1611 May 2, 2023 •

edited

Loading